feat: generate adjusted font fallbacks for smoother font loading #959
Conversation
commit: |
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
benlife5
force-pushed
the
preload-fonts
branch
from
e653e07 to
029fbe4
Compare
WalkthroughReorganized visual-editor font utilities into a new utils/fonts/ subdirectory, added Capsize dependencies and a generateFontFallbacks script that emits fallback font-face JSON, and adjusted font_registry weight ranges. Default font-family strings were extended to include explicit fallback entries. Import paths across components were updated to the new font module locations. applyTheme now collects and prepends generated fallback font-face CSS before theme styles and font links. Mapbox iframe loader no longer injects font loading. Tests and test utilities were updated to reflect fallback changes and extraction logic. Sequence Diagram(s)mermaid mermaid Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @packages/visual-editor/src/utils/applyTheme.ts:
- Around line 98-107: The loop over Object.keys(inUseGoogleFonts) assumes
fontFallbackTransformations[fontFamily] exists and will throw if missing; update
the block in applyTheme.ts to check for the key before using it (e.g. if
(!fontFallbackTransformations[fontFamily]) skip or default to an empty array),
and only push joined strings into fallbackFontFaceDefinitions when a valid array
is present; optionally emit a console.warn or logger message referencing
fontFamily when a mapping is missing.
In @packages/visual-editor/src/utils/fonts/generateFontFallbacks.ts:
- Around line 66-69: The current replacement that builds finalizedFontFace by
doing fontFaces.replace("}", ` font-weight: ${weight};\n font-style:
${style};\n}`) is fragile; update generateFontFallbacks to safely inject the
font-weight and font-style into the final @font-face block produced by
createFontStack: locate the last closing brace with a robust method (e.g., a
regex that matches the final `}` of the top-level @font-face block or parse the
CSS string to find the block boundaries), validate that a single @font-face
block was found, and then insert `font-weight` and `font-style` just before that
final brace (or throw/log a clear error if the expected format isn't present);
reference the variables fontFaces, finalizedFontFace, createFontStack, weight,
and style when making the change.
In @packages/visual-editor/src/utils/fonts/visualEditorFonts.ts:
- Line 3: Update the import in visualEditorFonts.ts to use a relative
same-directory path: replace the current import of defaultFonts as fontsJs from
"../fonts/font_registry.js" with an import from "./font_registry.js" so it
correctly references the font_registry module in the same folder.
🧹 Nitpick comments (5)
packages/visual-editor/src/utils/fonts/font_registry.js (1)
3-4567: Variable font weight range normalization indefaultFontslooks saneThe updated
minWeight/maxWeightvalues for the variable faces (e.g., Cairo, DM Sans, Mulish, Readex Pro, Recursive, Roboto Flex, Sofia Sans*, Ysabeau*, Wavefont, Linefont, etc.) all fall into a consistent 100–900‑style range and are coherent with typical Google Fonts variable ranges. Given this file is auto‑generated, the main thing is to ensure the generator and any consumers (e.g.,extractInUseFontFamilies, fallback generation, link tag construction) are using the same source of truth, and that you have at least one test that fails ifdefaultFontsanddefaultGoogleFontsLinkTagsever drift structurally.packages/visual-editor/src/utils/fonts/generateFontFallbacks.ts (4)
5-13: Consider using dynamic imports consistently.The static imports of system fallback metrics require
@ts-ignoredue to module resolution issues. Since dynamic imports are already used for font-specific metrics (line 62), consider loading all metrics dynamically for consistency and to avoid TypeScript suppressions.🔎 Example refactor using dynamic imports
-// Requires different TS module resolution than we're using -//@ts-ignore -import arial from "@capsizecss/metrics/arial"; -//@ts-ignore -import georgia from "@capsizecss/metrics/georgia"; -//@ts-ignore -import courierNew from "@capsizecss/metrics/courierNew"; -//@ts-ignore -import brushScript from "@capsizecss/metrics/brushScript"; - -const systemFallbacks: Record<string, any> = { - "sans-serif": arial, - serif: georgia, - cursive: brushScript, - monospace: courierNew, -}; +const systemFallbackPaths: Record<string, string> = { + "sans-serif": "@capsizecss/metrics/arial", + serif: "@capsizecss/metrics/georgia", + cursive: "@capsizecss/metrics/brushScript", + monospace: "@capsizecss/metrics/courierNew", +};Then load them dynamically in the generate function:
const fallbackMetricsPath = systemFallbackPaths[config.fallback] || systemFallbackPaths["sans-serif"]; const { default: fallbackMetrics } = await import(fallbackMetricsPath);
34-34: Validate fallback key exists in systemFallbacks.The code falls back to
arialifconfig.fallbackis not found, but this happens silently. Add validation to ensure all fallback keys in the font registry are supported.🔎 Proposed validation
const camelName = fontFamilyToCamelCase(displayName); - const fallbackMetrics = systemFallbacks[config.fallback] || arial; + if (!systemFallbacks[config.fallback]) { + console.warn( + `Unknown fallback type "${config.fallback}" for font "${displayName}", using "sans-serif" (arial)` + ); + } + const fallbackMetrics = systemFallbacks[config.fallback] || arial;
72-76: Catch block could provide more debugging context.The catch silently suppresses all import errors. Consider logging the error message to help debug issues with missing metrics or other import failures.
🔎 Enhanced error logging
- } catch { + } catch (error) { console.warn( - `Could not find metrics for ${displayName} (${variantSuffix})` + `Could not find metrics for ${displayName} (${variantSuffix}): ${error instanceof Error ? error.message : String(error)}` ); }
28-82: Add top-level error handling and usage instructions.The
generate()function is called immediately but has no error handling at the top level. If the function throws, the script will crash without helpful context. Additionally, document how to run this script and where to save the output.🔎 Enhanced script with error handling and documentation
/** * This script generates a JSON mapping of font families to the corresponding * font transformations that minimizes layout shift during font loading. * * It only needs to be run if we update font_registry.js. + * + * Usage: + * node --loader ts-node/esm generateFontFallbacks.ts > fontFallbackTransformations.json + * Or: + * tsx generateFontFallbacks.ts > fontFallbackTransformations.json */ async function generate() { const results: Record<string, string[]> = {}; for (const [displayName, config] of Object.entries(defaultFonts)) { // ... existing code ... } console.log(JSON.stringify(results, null, 2)); } -generate(); +generate().catch((error) => { + console.error("Failed to generate font fallbacks:", error); + process.exit(1); +});
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/visual-editor/package.jsonpackages/visual-editor/src/components/DefaultThemeConfig.test.tspackages/visual-editor/src/components/DefaultThemeConfig.tspackages/visual-editor/src/editor/Editor.tsxpackages/visual-editor/src/internal/types/templateMetadata.tspackages/visual-editor/src/internal/utils/loadMapboxIntoIframe.tsxpackages/visual-editor/src/utils/applyTheme.test.tspackages/visual-editor/src/utils/applyTheme.tspackages/visual-editor/src/utils/fonts/fontFallbackTransformations.jsonpackages/visual-editor/src/utils/fonts/font_registry.jspackages/visual-editor/src/utils/fonts/generateFontFallbacks.tspackages/visual-editor/src/utils/fonts/visualEditorFonts.test.tspackages/visual-editor/src/utils/fonts/visualEditorFonts.tspackages/visual-editor/src/utils/index.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/visual-editor/src/utils/fonts/generateFontFallbacks.ts (4)
packages/visual-editor/src/utils/fonts/font_registry.js (2)
defaultFonts(3-4567)defaultFonts(3-4567)packages/visual-editor/src/utils/fonts/visualEditorFonts.ts (1)
defaultFonts(25-25)packages/visual-editor/src/utils/index.ts (1)
defaultFonts(42-42)packages/visual-editor/scripts/generateFontRegistry.js (1)
weights(80-86)
packages/visual-editor/src/utils/applyTheme.ts (1)
packages/visual-editor/src/utils/fonts/visualEditorFonts.ts (1)
fontLinkDataToHTML(123-136)
packages/visual-editor/src/utils/applyTheme.test.ts (2)
packages/visual-editor/src/utils/applyTheme.ts (1)
applyTheme(46-115)packages/visual-editor/src/utils/index.ts (1)
applyTheme(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (12)
packages/visual-editor/src/internal/utils/loadMapboxIntoIframe.tsx (2)
35-35: Dependency array correctly simplified.The dependency array now correctly includes only
document, which is the sole external value used in the effect body after font loading was removed.
14-35: No action needed. Fonts are properly loaded into the iframe document through theupdateThemeInEditorfunction inapplyTheme.ts, which uses a MutationObserver to inject font links intoiframe.contentDocument.headafter the preview iframe mounts. The removal fromloadMapboxIntoIframe.tsxis a safe refactoring that consolidates font loading logic with other theme application code.packages/visual-editor/package.json (1)
109-111: Capsize devDependencies placement looks correctAdding
@capsizecss/coreand@capsizecss/metricsas devDependencies is appropriate for build‑time font‑fallback generation, assuming they are not imported from code that ships indist. Please just double‑check they’re only used from scripts/tooling, not runtime library entrypoints.packages/visual-editor/src/internal/types/templateMetadata.ts (1)
1-23: FontRegistry import path now aligned with new fonts/ layoutUpdating
FontRegistryto import from../../utils/fonts/visualEditorFonts.tskeepsTemplateMetadata.customFontswired to the moved font utilities and is consistent with the rest of the reorg. No behavioral change and no issues from this change alone.packages/visual-editor/src/utils/fonts/visualEditorFonts.test.ts (1)
1-8: ThemeData import path update is correct for the new directory structureThe
ThemeDataimport now correctly walks up two levels intointernal/types, matching this file’s new location underutils/fonts. Test behavior and coverage are unchanged.packages/visual-editor/src/editor/Editor.tsx (1)
25-28: Editor font utilities now sourced from the new fonts/ moduleSwitching
defaultFontsandloadFontsIntoDOMto../utils/fonts/visualEditorFonts.tskeeps the theme‑mode font‑loading effect wired to the relocated font utilities without changing behavior. TheuseEffectconditions and arguments remain consistent with the previous implementation.packages/visual-editor/src/components/DefaultThemeConfig.ts (1)
65-65: LGTM! Consistent fallback font additions across all theme elements.The addition of
'Open Sans Fallback'to all font family defaults is consistent and aligns with the new font fallback architecture introduced in this PR.Also applies to: 98-98, 131-131, 164-164, 197-197, 230-230, 263-263, 315-315, 362-362
packages/visual-editor/src/components/DefaultThemeConfig.test.ts (1)
3-3: LGTM! Tests properly updated to reflect fallback font behavior.The import path correction and updated test expectations correctly validate the new font fallback format. All assertions now verify that font values include the explicit fallback variant (e.g.,
'Custom Font', 'Custom Font Fallback', sans-serif).Also applies to: 21-21, 41-43, 60-60, 65-65
packages/visual-editor/src/utils/index.ts (1)
46-46: LGTM! Clean module reorganization.The export path update correctly reflects the relocation of font utilities into the
fonts/subdirectory.packages/visual-editor/src/utils/fonts/visualEditorFonts.ts (1)
33-33: LGTM! Fallback font correctly added to font stack.The font select option value now properly includes the fallback font variant in the font stack:
'${fontName}', '${fontName} Fallback', ${fontDetails.fallback}. This creates a proper cascade where the browser will attempt the primary font, then the generated fallback font-face, and finally the generic fallback family.packages/visual-editor/src/utils/applyTheme.test.ts (1)
89-114: LGTM! Tests properly validate font fallback injection.The test expectations correctly reflect the new behavior where:
- Font family values now include explicit fallback variants
- @font-face definitions for fallback fonts are injected before font link tags
- The CSS overrides include the full font stack with fallbacks
This properly validates the font preloading and fallback architecture introduced in this PR.
packages/visual-editor/src/utils/applyTheme.ts (1)
112-112: Verify the HTML injection order is intentional.The fallback font-face definitions are now injected before the actual font link tags. This ordering ensures fallback fonts are defined before the main fonts load, which should reduce layout shift. Confirm this is the desired behavior for optimal font loading.
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/components/testing/componentTests.setup.ts (1)
23-25: Clarify the condition check and document the magic index.The condition
matches[1]?.lengthis unusual and could be more explicit. Since you're checking for the presence of a second match,matches.length > 1would be clearer. Additionally, the hardcoded index1assumesapplyThemereturns font styles first and theme styles second, but this assumption isn't documented. If the output format ofapplyThemechanges, this test setup will break silently.♻️ Suggested improvements
// don't load fonts const matches = [...themeTags.matchAll(/<style[^>]*>([\s\S]*?)<\/style>/g)]; - if (matches.length > 0 && matches[1]?.length) { + // Skip first style block (font fallbacks) and use second block (theme) + if (matches.length > 1) { const theme = matches[1][0]; document.head.appendChild(tag); tag.outerHTML = theme; } else { console.error("failed to apply theme"); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/visual-editor/src/components/testing/componentTests.setup.tspackages/visual-editor/src/utils/applyTheme.tspackages/visual-editor/src/utils/fonts/visualEditorFonts.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/utils/applyTheme.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: call_unit_test / unit_tests (22.x)
- GitHub Check: call_unit_test / unit_tests (24.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
- GitHub Check: create-dev-release
🔇 Additional comments (3)
packages/visual-editor/src/utils/fonts/visualEditorFonts.ts (3)
1-5: LGTM! Import paths correctly updated for new file location.The relative import paths have been properly updated to reflect the file's new location in the
utils/fonts/subdirectory. All paths are correct and maintain proper module resolution.
303-337: LGTM! Function correctly handles the new font-family format.The
extractInUseFontFamiliesfunction splits the font-family value by comma and extracts the first font name (lines 317-318). This logic continues to work correctly with the new format that includes fallback fonts, as it properly extracts only the primary font name for loading. The generated fallback fonts don't need to be included in theinUseGoogleFontsorinUseCustomFontsresults since they're defined via generated CSS rather than external font files.
27-37: Fallback font naming convention is properly generated and loaded.The fallback fonts with the naming pattern
'${fontName} Fallback'are generated viagenerateFontFallbacks.ts, stored infontFallbackTransformations.json, and injected into the document head viaapplyThemebefore the font-family declarations are used. The test suite confirms this behavior with examples like'Adamina Fallback'and'Open Sans Fallback'. The browser gracefully handles the font stack through CSS's built-in fallback mechanism.
benlife5
requested review from
asanehisa,
briantstephan,
colton-demetriou and
mkilpatrick
packages/visual-editor/src/utils/fonts/generateFontFallbacks.ts
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/utils/fonts/generateFontFallbacks.ts
Outdated
Show resolved
Hide resolved
asanehisa
left a comment
asanehisa
left a comment
There was a problem hiding this comment.
Oh wow the video looks so much better than our current font flashing! Great job!
benlife5
changed the title
Used capsizecss to generate fallback font transformations for each Google font. This significantly reduces layout shift when the fallback font is replaced with the downloaded font. There is script in the library to generate these font face declarations so that the full capcize package does not need to be installed in the starter. https://smartly-brilliant-gerbil.dev.pgsdemo.com/ny/new-york/99-9th-avenue https://github.com/user-attachments/assets/a8388074-7885-4fea-9a17-f5e5c8591828 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Used capsizecss to generate fallback font transformations for each Google font. This significantly reduces layout shift when the fallback font is replaced with the downloaded font. There is script in the library to generate these font face declarations so that the full capcize package does not need to be installed in the starter.
https://smartly-brilliant-gerbil.dev.pgsdemo.com/ny/new-york/99-9th-avenue
Screen.Recording.2026-01-06.at.4.56.42.PM.mov